[HTTP/2] Functional Test cases#2367
Conversation
reta
force-pushed
the
issue-2125
branch
6 times, most recently
from
d3a8988 to
e248c6e
Compare
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Codecov Report
@@ Coverage Diff @@
## main #2367 +/- ##
============================================
- Coverage 61.09% 61.04% -0.05%
+ Complexity 3271 3267 -4
============================================
Files 260 260
Lines 18363 18362 -1
Branches 3250 3250
============================================
- Hits 11219 11210 -9
- Misses 5559 5563 +4
- Partials 1585 1589 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@peternied I have added a few new tests cases but the major change is to move the test client to Apache Async HttpClient 5 (which supports HTTP/2) and asserting that all communication happens using HTTP/2 protocol when HTTPS/SSL is on [1], thank you. |
| Assert.assertFalse(ccs.getBody().contains("AnotherSecredField")); | ||
| Assert.assertFalse(ccs.getBody().contains("xxx1")); Assert.assertEquals(ccs.getHeaders().toString(), 1, ccs.getHeaders().size()); | ||
| Assert.assertFalse(ccs.getBody().contains("xxx1")); | ||
| Assert.assertEquals(ccs.getHeaders().toString(), 2, ccs.getHeaders().size()); |
There was a problem hiding this comment.
There are 2 headers now: content type and content length
| return new SecureRestClientBuilder(settings, configPath).setSocketTimeout(60000).build(); | ||
| return new SecureRestClientBuilder(settings, configPath) | ||
| .setSocketTimeout(60000) | ||
| .setConnectionRequestTimeout(180000) |
There was a problem hiding this comment.
Should be fixed by opensearch-project/common-utils#287 but needs manual setting now
There was a problem hiding this comment.
Should we wait until that issue has been published? or we can create a tracker issue that reverts this change once common-utils publishes an artifact with that change
There was a problem hiding this comment.
No action is needed: this change is good, it may just be redundant after opensearch-project/common-utils#287 but keeping it is fine, thanks.
| final RequestConfig.Builder requestConfigBuilder = RequestConfig.custom() | ||
| .setResponseTimeout(Timeout.ofSeconds(60)); | ||
|
|
||
| return hcb.setDefaultRequestConfig(requestConfigBuilder.build()).disableAutomaticRetries().build(); |
There was a problem hiding this comment.
Was disableAutomaticRetries() added because of issues with any test?
Leaving a link to the Default class in HttpComponents-client5 to come back to later to see the list of default non-retriable and retriable codes: https://hc.apache.org/httpcomponents-client-5.1.x/current/httpclient5/apidocs/org/apache/hc/client5/http/impl/DefaultHttpRequestRetryStrategy.html
Create the HTTP request retry strategy with a max retry count of 1, default retry interval of 1 second, and using the following list of non-retriable I/O exception classes:
InterruptedIOException UnknownHostException ConnectException ConnectionClosedException SSLException and retriable HTTP status codes:
SC_TOO_MANY_REQUESTS (429) SC_SERVICE_UNAVAILABLE (503)
There was a problem hiding this comment.
Was
disableAutomaticRetries()added because of issues with any test?Leaving a link to the Default class in HttpComponents-client5 to come back to later to see the list of default non-retriable and retriable codes: https://hc.apache.org/httpcomponents-client-5.1.x/current/httpclient5/apidocs/org/apache/hc/client5/http/impl/DefaultHttpRequestRetryStrategy.html
Create the HTTP request retry strategy with a max retry count of 1, default retry interval of 1 second, and using the following list of non-retriable I/O exception classes:
InterruptedIOException UnknownHostException ConnectException ConnectionClosedException SSLException and retriable HTTP status codes:
SC_TOO_MANY_REQUESTS (429) SC_SERVICE_UNAVAILABLE (503)
Not really in the security plugin tests, but it is not needed (enabling retries won't harm here)
| } | ||
|
|
||
|
|
||
| @Test |
There was a problem hiding this comment.
Curious, is there a way for a cluster admin to force HTTP/1? Would that ever be valid or desired as a config setting?
There was a problem hiding this comment.
Hm ... this is a good question, I don't see why one would do that intentionally, but the RestClientBuilder could force HTTP/1.1, see please [1] fe
4 participants
Description
Add functional test cases for HTTP/2
Issues Resolved
Closes #2125
Testing
Added new test cases
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.